tests: Add mock scenarios for mcp23009e driver.#203
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #200 by expanding mock scenario coverage for the mcp23009e driver, bringing it closer to parity with other drivers’ mock test coverage and exercising more of the driver’s public API via the YAML scenario runner.
Changes:
- Added mock сценарios validating IODIR/GPIO register read-write helpers (
get_*/set_*). - Added mock coverage for
setup()across multiple configurations (direction, pull-up, polarity inversion). - Added mock coverage for per-pin level control (
set_level()/get_level()) and reset-pin power management (power_off()/power_on()).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nedseb
left a comment
nedseb
left a comment
There was a problem hiding this comment.
Salut Charly ! Bonne première contribution, les tests sont bien pensés et couvrent les bonnes fonctionnalités. Voici une revue détaillée pour t'aider à progresser.
Référence issue
La PR indique Solve issue : #200 — la convention du projet est d'utiliser Closes #200 dans la description. C'est important car GitHub ferme automatiquement l'issue quand la PR est mergée. "Solve" n'est pas un mot-clé reconnu par GitHub, donc l'issue resterait ouverte.
Message de commit
Le message tests: Add mock scenarios for mcp23009e driver. est parfait ✅ — il respecte exactement le format <scope>: <Description.> du projet. Bien joué.
Tests — remarques par scénario
1. "Set IODIR register" ✅
Bien, simple et efficace.
2. "Set and read full GPIO port" ✅
Bon test. Petite remarque : le mock initialise GPIO à 0xF0 (ligne 39 du YAML). Quand tu écris 0x5A, le FakeI2C écrase la valeur. C'est correct mais ça vaut le coup de le noter en commentaire pour la lisibilité.
3-5. Tests setup() ✅✅✅
Très bien pensés — tu testes les 3 sous-registres (IODIR, GPPU, IPOL) en un seul test. C'est exactement le pattern attendu.
Remarque importante : dans ces tests, tu appelles dev.set_iodir(0x00), dev.set_gppu(0x00), dev.set_ipol(0x00) AVANT setup() pour initialiser l'état. C'est correct, mais le driver setup() fait un read-modify-write : il lit les registres avec _read_reg(), modifie le bit, puis écrit. Ton test vérifie donc bien le comportement de modification de bit individuel.
6-7. "Set and get individual output level" ✅✅
Bon test. Tu vérifies bien que set_level() modifie uniquement le bit ciblé.
8. "Set level does not modify input pin" ⚠️
Très bon scénario — tu testes le guard if _get_bit(iodir, gpx) == MCP_DIR_INPUT: return. Cependant, il y a une incohérence subtile :
dev.set_iodir(0xFF) # Tous les pins en INPUT
dev.set_gpio(0xF0) # Force GPIO à 0xF0
dev.set_level(4, 0) # Essaie de mettre GP4 à 0 → devrait être ignoré
result = dev.get_gpio() == 0xF0 and dev.get_level(4) == 1Tu vérifies que get_gpio() == 0xF0 (le set_level n'a rien changé) ✅, mais tu vérifies aussi que get_level(4) == 1. get_level(4) lit le registre GPIO et extrait le bit 4. 0xF0 = 0b11110000, le bit 4 est bien à 1. C'est correct mais le commentaire dans le nom du test devrait expliquer pourquoi — ce n'est pas évident au premier coup d'œil.
9. "Get GPPU register after write" ✅
Simple et efficace.
10. "Power cycle toggles reset pin" ✅
Bon test, mais c'est partiellement redondant avec les tests existants "Power off holds reset pin low" (ligne 56) et "Power on releases reset pin" (ligne 64). Ton test combine les deux en un seul scénario, ce qui a l'avantage de vérifier la séquence off→on. C'est acceptable mais note qu'il ne teste rien de nouveau fonctionnellement.
Ce qui manque
L'issue #200 demandait au moins 8 tests. Tu en as ajouté 10, c'est bien ✅. Mais voici des scénarios qui auraient été intéressants à couvrir et qui ne sont pas encore testés :
set_level()avec pin > 7 — vérifier que le guardif gpx > 7: returnfonctionneget_level()avec pin > 7 — devrait retournerMCP23009_LOGIC_LOWreset()— vérifier que les registres reviennent à leur valeur par défaut après un resetset_olat()/get_olat()— registre output latch, non testé- Interrupts —
interrupt_on_change(),interrupt_on_falling(),interrupt_on_raising(),disable_interrupt()ne sont testés qu'en hardware. Des tests mock vérifieraient que les registres GPINTEN/INTCON sont bien configurés.
Ce n'est pas bloquant pour cette PR mais ce serait de bonnes améliorations à ajouter dans un second temps.
Style YAML
Le code est propre et bien formaté. Deux suggestions mineures :
- Ajouter des commentaires de section comme dans les tests existants. Par exemple, avant tes tests tu pourrais ajouter :
# --- Register access tests ---Regarde les commentaires # --- Polling tests --- et # --- Interrupt tests --- déjà présents dans le fichier pour le pattern.
- Les assertions composées avec
andsont lisibles mais si l'une échoue, le message d'erreur ne dit pas laquelle. Ce n'est pas critique mais pour les cas complexes, séparer en plusieurs tests peut aider au debug.
Résumé
| Aspect | Évaluation |
|---|---|
| Commit message | ✅ Parfait |
| Référence issue | Closes #200 au lieu de Solve issue : #200 |
| Nombre de tests | ✅ 10 ajoutés (8 demandés) |
| Qualité des tests | ✅ Bien pensés, bonne couverture |
| Redondance | |
| Tests manquants | 💡 Bounds checking, reset, interrupts mock |
| Style YAML | 💡 Ajouter des commentaires de section |
Pour merger : corrige la référence issue dans la description (Closes #200), et ajoute des commentaires de section dans le YAML. Le reste sont des améliorations optionnelles.
|
Quelques remarques complémentaires : Référence issueTu références Tests supplémentaires à considérer
Amélioration d'un test existantLe test existant "Read GPIO register" (ligne 50) utilise Description de PRLa sortie complète de pytest (157 tests) est collée deux fois dans la description. Pour les futures PRs, inclure uniquement les tests concernés par ta modification suffit (la section |
|
Add Constructor test and soft reset test Should be ready to merge |
|
Charly, tu dis que tout est traité mais en vérifiant point par point : Revue initiale — suivi
Problèmes restants1. ❌ Description de la PR toujours incorrecteToujours 2. ❌ Pas de newline finaleLe fichier se termine encore par 3. ❌ Suppression du mode hardware sur "Read GPIO register"Tu as changé : # Avant
mode: [mock, hardware]
# Après
mode: [mock]Ce test fonctionnait aussi en hardware (il lit un registre réel). Pourquoi l'avoir retiré ? Si c'est involontaire, remet 4. Commentaire de réponse insuffisantTu as écrit :
En pratique, tu dois répondre à chaque point de la revue pour que le reviewer sache ce qui a été traité et ce qui ne l'a pas été. "Should be ready to merge" sans vérifier la newline et la référence issue montre que tu n'as pas relu la revue en entier. Corrige les 3 points restants (description PR, newline finale, mode hardware du test GPIO) et ce sera bon. |
|
Les 3 corrections sont bonnes ✅ — la PR est prête à merger. Juste une clarification importante pour la suite : Issue ≠ Pull RequestDans ta réponse tu écris "la PR #200" et "la PR #181". Ce sont des issues, pas des PRs. C'est important de distinguer les deux :
Dans le GitHub Flow : Ici, #200 est l'issue (le ticket), et #203 est ta PR (le code). Retiens : les issues ont des numéros, les PR aussi, mais ce ne sont pas les mêmes objets. Pour vérifier : va sur |
nedseb
left a comment
nedseb
left a comment
There was a problem hiding this comment.
Les 3 corrections sont bonnes ✅ — la PR est prête à merger.
|
🎉 This PR is included in version 0.0.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes #200
Description
Add additional mock tests for the
mcp23009edriver to improve test coverage.Previously, the driver had only a few mock tests compared to other drivers. This PR adds tests covering register access, pin configuration, GPIO control, and power management.
Changes
Added 10 new mock tests in
tests/scenarios/mcp23009e.yamlCovered methods:
get_iodir()/set_iodir()get_gpio()/set_gpio()setup()(input, output, pull-up, polarity)set_level()/get_level()get_gppu()power_off()/power_on()mock test results
Checklist
pytest tests/ -k "mock and mcp23009e"passespytest tests/ -k mock)